Closes #372 - Add Distribution for tasks #373

jannikFuellgrafEnvite · 2025-01-07T10:59:35Z

Thanks for your PR! Please fill out the following list :)

I put the ticket or multiple tickets in review
Commit message format → Closes #<Issue Number> - Your commit message.
Sonarcloud link : https://sonarcloud.io/summary/new_code?id=jannikFuellgrafEnvite_kadai&branch=kad%2Fdistribute-tasks-no-transfer-flag-change
No documentation update needed
Link to PR with documentation update: Add Distribution SPI kadai-doc#43
No Release Notes needed
Release Notes :

lib/kadai-core/src/main/java/io/kadai/spi/task/internal/TaskDistributionManager.java

gitgoodjhe · 2025-01-24T09:00:58Z

lib/kadai-test-api/src/main/java/io/kadai/testapi/DefaultTestEntities.java

@@ -52,6 +56,16 @@ public static WorkbasketBuilder defaultTestWorkbasket() {
        .orgLevel1("company");
  }

+  public static TaskBuilder defaultTask(


Let's talk about this

gitgoodjhe · 2025-01-24T09:08:49Z

rest/kadai-rest-spring/src/main/java/io/kadai/task/rest/TaskController.java

+    // Distribute tasks based on the provided parameters
+    BulkOperationResults<String, KadaiException> result;
+
+    if (destinationWorkbasketIds != null && distributionStrategyName != null) {


maybe we can refactor this in a separate method, let's discuss :)

gitgoodjhe · 2025-01-31T12:43:10Z

lib/kadai-core/src/main/java/io/kadai/spi/task/internal/TaskDistributionManager.java

+
+    if (taskDistributionProviderList.isEmpty()) {
+      LOGGER.info(
+          "No Custom TaskDistribution Provider found. Running wit DefaultTaskDistributionProvider");


Suggested change

"No Custom TaskDistribution Provider found. Running wit DefaultTaskDistributionProvider");

"No Custom TaskDistribution Provider found. Running with DefaultTaskDistributionProvider");

gitgoodjhe · 2025-01-31T12:46:19Z

rest/kadai-rest-spring/src/main/java/io/kadai/task/rest/TaskController.java

+   *       <li>{@code sourceWorkbasketId} ({@code String}): The ID of the source workbasket. Must be
+   *           set if {@code taskIds} is null.
+   *       <li>{@code taskIds} ({@code List<String>}): A list of task IDs to be distributed. Must be
+   *           set if {@code sourceWorkbasketId} is null. The other parameter must be null to ensure


Is this description still correct considering the sourceWorkasbasketId must now always be provided? Refers to complete description, e.g. line 1079,1112 as well etc.

gitgoodjhe · 2025-01-31T12:49:27Z

rest/kadai-rest-spring/src/main/java/io/kadai/task/rest/TaskController.java

+  @PostMapping(path = RestEndpoints.URL_DISTRIBUTE)
+  @Transactional(rollbackFor = Exception.class)
+  public ResponseEntity<BulkOperationResultsRepresentationModel> distributeTasks(
+      @Valid @RequestBody DistributionTasksRepresentationModel distributionTasksRepresentationModel,


Do we need the @Valid here? I don't see the Model expressing any constraints. Would it even work?

gitgoodjhe · 2025-01-31T13:01:13Z

lib/kadai-core/src/main/java/io/kadai/task/internal/TaskDistributor.java

+      if (taskIds.isEmpty()) {
+        return operationResults;
+      } else {
+        checkIfTasksInSameWorkbasket(taskIds);


these two checks will also be performed, if no taskIds are provided and retrieved via the sourceWorkbasket, but we don't need it in this case, right?

lib/kadai-core/src/main/java/io/kadai/task/internal/TaskDistributor.java

holgerhagen · 2025-02-13T14:07:41Z

lib/kadai-core/src/main/java/io/kadai/spi/task/internal/TaskDistributionManager.java

+    } else {
+      LOGGER.info("No distribution strategy specified. Using default distribution.");
+      newTaskDistribution =
+          new DefaultTaskDistributionProvider()


why don't you add the default provider to the list of provider? In that case you don't even need a if-then-else block?

holgerhagen · 2025-02-13T14:12:15Z

lib/kadai-core/src/main/java/io/kadai/task/api/TaskService.java

+   *     workbasket.
+   * @param destinationWorkbasketIds A list of {@linkplain Workbasket#getId() Ids} of the
+   *     destination workbaskets where tasks will be distributed.
+   * @param distributionStrategyName The name of the custom distribution strategy to be applied. The


The strategy name is always the full qualified class name?

For the class, the simple name is sufficient. I have adjusted and specified the description accordingly.

lib/kadai-core/src/main/java/io/kadai/task/api/TaskService.java

lib/kadai-core-test/src/test/java/acceptance/task/distribute/DistributeTaskAccTest.java

lib/kadai-core/src/main/java/io/kadai/spi/task/internal/DefaultTaskDistributionProvider.java

CRoberto1926 · 2025-02-14T10:19:47Z

rest/kadai-rest-spring/src/main/java/io/kadai/task/rest/TaskController.java

+    if (taskIds == null) {
+      if (destinationWorkbasketIds != null && distributionStrategyName != null) {
+        result =
+            taskService.distribute(
+                workbasketId,
+                destinationWorkbasketIds,
+                distributionStrategyName,
+                additionalInformation);
+      } else if (destinationWorkbasketIds != null) {
+        result = taskService.distributeWithDestinations(workbasketId, destinationWorkbasketIds);
+      } else if (distributionStrategyName != null) {
+        result =
+            taskService.distributeWithStrategy(
+                workbasketId, distributionStrategyName, additionalInformation);
+      } else {
+        result = taskService.distribute(workbasketId);
+      }
+    } else {
+      if (destinationWorkbasketIds != null && distributionStrategyName != null) {
+        result =
+            taskService.distribute(
+                workbasketId,
+                taskIds,
+                destinationWorkbasketIds,
+                distributionStrategyName,
+                additionalInformation);
+      } else if (destinationWorkbasketIds != null) {
+        result =
+            taskService.distributeWithDestinations(workbasketId, taskIds, destinationWorkbasketIds);
+      } else if (distributionStrategyName != null) {
+        result =
+            taskService.distributeWithStrategy(
+                workbasketId, taskIds, distributionStrategyName, additionalInformation);
+      } else {
+        result = taskService.distribute(workbasketId, taskIds);
+      }
+    }


I would suggest to extract this in a private method and simplify the if else chain, so that all possibilities are more clearly expressed in code.

CRoberto1926

Sieht gut aus 🙂

holgerhagen requested changes Jan 14, 2025

View reviewed changes

lib/kadai-core/src/main/java/io/kadai/spi/task/internal/TaskDistributionManager.java Show resolved Hide resolved

jannikFuellgrafEnvite force-pushed the kad/distribute-tasks-no-transfer-flag-change branch 10 times, most recently from da4edc2 to 36d24f4 Compare January 22, 2025 07:14

gitgoodjhe requested changes Jan 24, 2025

View reviewed changes

jannikFuellgrafEnvite force-pushed the kad/distribute-tasks-no-transfer-flag-change branch 6 times, most recently from 10ded54 to 8316c7e Compare January 28, 2025 11:00

jannikFuellgrafEnvite linked an issue Jan 29, 2025 that may be closed by this pull request

Add Distribution for Tasks #372

Closed

gitgoodjhe requested changes Jan 31, 2025

View reviewed changes

jannikFuellgrafEnvite force-pushed the kad/distribute-tasks-no-transfer-flag-change branch 3 times, most recently from 86a9be0 to 9a24ce9 Compare February 3, 2025 15:02

gitgoodjhe reviewed Feb 7, 2025

View reviewed changes

lib/kadai-core/src/main/java/io/kadai/task/internal/TaskDistributor.java Show resolved Hide resolved

jannikFuellgrafEnvite force-pushed the kad/distribute-tasks-no-transfer-flag-change branch from 9a24ce9 to 77441b0 Compare February 12, 2025 16:09

gitgoodjhe approved these changes Feb 13, 2025

View reviewed changes

jannikFuellgrafEnvite force-pushed the kad/distribute-tasks-no-transfer-flag-change branch from 77441b0 to 1df144f Compare February 13, 2025 13:45

jannikFuellgrafEnvite requested a review from holgerhagen February 13, 2025 13:49

holgerhagen requested changes Feb 13, 2025

View reviewed changes

jannikFuellgrafEnvite force-pushed the kad/distribute-tasks-no-transfer-flag-change branch from 1df144f to e6d9315 Compare February 13, 2025 16:37

jannikFuellgrafEnvite requested a review from holgerhagen February 14, 2025 07:30

CRoberto1926 reviewed Feb 14, 2025

View reviewed changes

jannikFuellgrafEnvite force-pushed the kad/distribute-tasks-no-transfer-flag-change branch from e6d9315 to 87ed38a Compare February 24, 2025 10:27

Closes kadai-io#372 - Add Distribution for tasks

a7f9c9f

jannikFuellgrafEnvite force-pushed the kad/distribute-tasks-no-transfer-flag-change branch from 87ed38a to a7f9c9f Compare February 24, 2025 10:30

jannikFuellgrafEnvite requested a review from CRoberto1926 February 24, 2025 11:09

CRoberto1926 reviewed Feb 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #372 - Add Distribution for tasks #373

Closes #372 - Add Distribution for tasks #373

jannikFuellgrafEnvite commented Jan 7, 2025 •

edited

Loading

gitgoodjhe Jan 24, 2025

gitgoodjhe Jan 24, 2025

gitgoodjhe Jan 31, 2025

gitgoodjhe Jan 31, 2025

gitgoodjhe Jan 31, 2025

gitgoodjhe Jan 31, 2025

holgerhagen Feb 13, 2025

holgerhagen Feb 13, 2025

jannikFuellgrafEnvite Feb 13, 2025

CRoberto1926 Feb 14, 2025

CRoberto1926 left a comment

	"No Custom TaskDistribution Provider found. Running wit DefaultTaskDistributionProvider");
	"No Custom TaskDistribution Provider found. Running with DefaultTaskDistributionProvider");

Closes #372 - Add Distribution for tasks #373

Are you sure you want to change the base?

Closes #372 - Add Distribution for tasks #373

Conversation

jannikFuellgrafEnvite commented Jan 7, 2025 • edited Loading

Thanks for your PR! Please fill out the following list :)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CRoberto1926 left a comment

Choose a reason for hiding this comment

jannikFuellgrafEnvite commented Jan 7, 2025 •

edited

Loading